feat: user configurable tui keybindings#2415
Conversation
f9fd084 to
ac8aadf
Compare
6d04cff to
dd0d120
Compare
|
/review Any thoughts @krissetto ? |
aheritier
left a comment
There was a problem hiding this comment.
Good foundation for configurable keybindings. The architecture (centralized KeyMap, defaults + override merge, caching) is sound and the refactoring of tui.go to use the shared KeyMap is a clear improvement.
Blocking items:
- Type naming:
Keybindings→Keybinding(singular for a single entry) - Undocumented behavioral change: The quit key now opens an exit confirmation dialog — this changes existing UX and should be explicitly noted/discussed
- Missing docs: No user-facing documentation for how to configure keybindings
CI: Still running at review time — please confirm tests pass.
Also noting (non-blocking): PR title has a typo — "keybings" → "keybindings".
|
❌ PR Review Failed — The review agent encountered an error and could not complete the review. View logs. |
dd0d120 to
9642489
Compare
|
@aheritier this can be re-reviewed 👍 |
|
/review |
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
This PR introduces user-configurable TUI keybindings — a clean, well-structured implementation with good test coverage. Three issues were found in the new code:
- Data race in
ResetKeys()— writeskeysOncewithout a mutex whileGetKeys()reads it concurrently (flagged by Go's race detector, and blocks safe hot-reload use). - Conflict detection not enforced in
validateKeys— conflicting keys are logged but still bound to both actions, leading to ambiguous key dispatch. - Elicitation dialog exits without confirmation — the quit key in
ElicitationDialog.handleKeyPresscallstea.Quitdirectly rather than stacking the exit confirmation dialog like every other quit path does.
A low-severity edge case was also noted: if a user maps quit to n/N, the exit confirmation dialog's Yes binding would capture it before the No binding.
4f08c66 to
038379a
Compare
As referenced here: #1626 and the subsequent closed MR: #1629
First stage implementation for allowing user configurable keybindings.
@krissetto would like to get your thoughts and then we can discuss potential extensions/alternative implementations.